Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move hash based actions to be event triggered #395

Merged
merged 2 commits into from
Oct 13, 2020
Merged

Conversation

creesch
Copy link
Member

@creesch creesch commented Oct 11, 2020

Done in preparation for #388. This PR moves toolbox's current hash processing to an event based system which allows us to easily hook in other things as well.

Initially I also wanted to implement things so that when a relative url is clicked to the settings that the state of the url is changed instead it reloading the page. However there is some fuckery going on with reddit urls making that a bit more difficult than I'd like. I briefly debated simply doing it for all a elements that open toolbox settings (and just ignoring whatever url they are pointing to) but I have no idea how people link to settings in regards to mod onboarding so I decided against that.

I'll do #388 in a separate PR once this is merged.

@eritbh
Copy link
Member

eritbh commented Oct 11, 2020

Initially I also wanted to implement things so that when a relative url is clicked to the settings that the state of the url is changed instead it reloading the page.

Almost wish there were a RES feature to revert Reddit's garbage link handling in all cases. I know it's a relatively new thing on old Reddit at least. There's a lot of on-the-fly href modification that happens in response to link clicks that ideally we'd be able to override, but I don't think it's worth it for us at this point.

Anyway... Before I dive into the code here, is there a reason fragments are being handled in a separate function from the existing URL change detection? My understanding is that the existing stuff should handle hash changes the same as any other part of the URL changing. Correct me if I'm wrong.

@creesch
Copy link
Member Author

creesch commented Oct 11, 2020

Anyway... Before I dive into the code here, is there a reason fragments are being handled in a separate function from the existing URL change detection?

Mostly because refreshUrlContext() already is fairly long and deals with actual page changes where this deals with hashes. So it made sense to me to separate the handling of the two to keep things readable.

I started out with all the logic within refreshUrlContext() but that was a bit messy, so I separated the hash specific stuff to refreshHashContext() and called that from within refreshUrlContext() and at that point I decided I might as well separate them entirely so they can also be worked on independently more easily if need be.

@creesch creesch requested a review from eritbh October 12, 2020 16:52
Copy link
Member

@eritbh eritbh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should consider renaming refreshUrlContext to something like refreshPathContext since it's not responsible for the whole URL anymore? Otherwise LGTM.

@creesch creesch merged commit ed6cd24 into master Oct 13, 2020
@creesch creesch deleted the event-hash-params branch October 13, 2020 08:41
@eritbh eritbh added this to the v5.5 milestone Oct 13, 2020
eritbh pushed a commit that referenced this pull request Sep 5, 2024
* Move hash based actions to be event triggered

* refreshUrlContext -> refreshPathContext
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants